fix: Add fetch for default branch #1031
Conversation
…t default branch during migration
google-cla
bot
added
the
cla: yes
There was a problem hiding this comment.
This is looking pretty reasonable to me, with the caveat that I think it would be good to add a test.
I think you could use something like requests-mock so that you don't need to actually care about the traffic to GitHub.
synthtool/gcp/common.py
Outdated
| @@ -353,3 +359,9 @@ def _load_repo_metadata(metadata_file: str = "./.repo-metadata.json") -> Dict: | |||
| with open(metadata_file) as f: | |||
| return json.load(f) | |||
| return {} | |||
|
|
|||
| def _get_default_branch_name(package_name: str) -> str: | |||
There was a problem hiding this comment.
It looks like this might not be used yet?
synthtool/gcp/common.py
Outdated
| @@ -58,6 +59,11 @@ def _generic_library(self, directory: str, **kwargs) -> Path: | |||
|
|
|||
| t = templates.TemplateGroup(self._template_root / directory, self.excludes) | |||
|
|
|||
| repo = kwargs["metadata"]["repository"] | |||
There was a problem hiding this comment.
This looks reasonable to me, but I think it would be worth adding a test here:
https://github.com/googleapis/synthtool/blob/master/tests/test_templates.py
synthtool/gcp/common.py
Outdated
| @@ -58,6 +59,11 @@ def _generic_library(self, directory: str, **kwargs) -> Path: | |||
|
|
|||
| t = templates.TemplateGroup(self._template_root / directory, self.excludes) | |||
|
|
|||
| repo = kwargs["metadata"]["repository"] | |||
| github_req = requests.get(f"https://api.github.com/repos/{repo}") | |||
There was a problem hiding this comment.
@SurferJeffAtGoogle Will this be problematic for owl-bot (as it's not hermetic)?
There was a problem hiding this comment.
I think Jeff is out til Friday? Is there anyone else we can ask about this? Or do you have thoughts on this?
There was a problem hiding this comment.
To me this seems like it's probably okay, as it will be applied in the post-processor, and will stay consistent for the repo itself?
billyjacobson
requested review from
SurferJeffAtGoogle and
a team
as code owners
bcoe
added
the
kokoro:force-run
kokoro-team
removed
the
kokoro:force-run
|
@billyjacobson thank you for adding this 👍 sorry it took a while to land, the beginning of the week was a bit swamped. |
| @@ -41,6 +43,14 @@ def test_handles_empty_string(): | |||
| assert decamelize("") == "" | |||
|
|
|||
|
|
|||
| def test_get_default_branch(): | |||
| with requests_mock.Mocker() as m: | |||
| m.get( | |||
There was a problem hiding this comment.
I really appreciate that you added a test for this 🆗
|
Sorry I didn't chime in on this sooner. This is causing @sofisl and I problems because we're trying to generate templated code for a new library, that doesn't have a corresponding github repo yet. With this PR, @bcoe and @billyjacobson Can we observe or encode the default branch without making network requests? |
|
I don't know a ton about this system, but that does sound somewhat
impossible, no? To get the default branch needs a network request at some
point, so maybe there is some other way we can handle this. Perhaps Ben has
some more insights than I do.
…On Mon, May 10, 2021 at 6:24 PM Jeffrey Rennie ***@***.***> wrote:
Sorry I didn't chime in on this sooner.
This is causing @sofisl <https://github.com/sofisl> and I problems
because we're trying to generate templated code for a new library, that
doesn't have a corresponding github repo yet.
With this PR, _generic_library() fetches data from the network *during
template generation*. That's not what I imagined it would ever do. That
also means it will fail whenever the network is flaky. It's very fragile.
@bcoe <https://github.com/bcoe> and @billyjacobson
<https://github.com/billyjacobson> Can we observe or encode the default
branch without making network requests?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#1031 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABL2ITYOCL6CCPBHE47UHU3TNBMLLANCNFSM42TKCYTA>
.
--
*• **Billy Jacobson*
*•* Developer Programs Engineer,
*• *Google Cloud Developer Relations
*•* 111 8th Ave, New York, NY 10011
<https://maps.google.com/?q=111+8th+Ave,+New+York,+NY+10011&entry=gmail&source=g>
|
Add fetch for default branch so autogen files can use the correct default branch during migration